refactor(api)!: make fluent Delaunay builders canonical - #499

tests/serialization_vertex_preservation.rs (1)

56-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Confusing .expect() panic message.

.expect("Tds construction succeeded") fires only when build() returns Err, so the panic will misleadingly read "Tds construction succeeded: ". Line 118 in this same file uses the correctly-phrased "Tds construction failed" for the analogous case.

✏️ Proposed fix

-        .expect("Tds construction succeeded");
+        .expect("Tds construction should succeed despite duplicate coordinates");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/serialization_vertex_preservation.rs` around lines 56 - 59, The
`.expect()` message in the `DelaunayTriangulation::builder(...).build()` test is
backwards and will produce a misleading panic if construction fails. Update the
expectation string in this test case to match the failure path, consistent with
the analogous check later in the same file, so the panic message clearly
indicates `build()` failed rather than succeeded.

src/delaunay/construction.rs (1)

5073-5078: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Optional: consolidate the duplicated DedupPolicy::Epsilon matching.

grid_cell_size_value and the per-vertex epsilon are derived from two separate matches on the same dedup_policy a few lines apart. Extracting a small helper (e.g., returning (epsilon, grid_cell_size)) would remove the risk of the two arms drifting apart on a future edit, but current behavior is correct and covered by tests.

Also applies to: 5092-5093

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/delaunay/construction.rs` around lines 5073 - 5078, The
DedupPolicy::Epsilon logic is duplicated when deriving grid_cell_size_value and
the per-vertex epsilon, so the two match expressions can drift apart over time.
Refactor the nearby matching in construction.rs around the dedup_policy handling
into a small helper that returns both values together, and update the code that
sets grid_cell_size_value and the epsilon assignment to use that shared result
while preserving the existing behavior for DedupPolicy::Epsilon,
DedupPolicy::Exact, and DedupPolicy::Off.

src/delaunay/builder.rs (1)

1667-1728: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Consider extracting shared Euclidean/Canonicalized validation logic to avoid drift between build_with_kernel and build_with_kernel_and_statistics.

The Euclidean/Canonicalized branches here (topology rejection, model validation, canonicalization) duplicate the corresponding logic in build_with_kernel (lines 1525-1556), differing only in calling build_with_kernel_options_and_statistics vs build_with_kernel_options. A future fix/change to one path (e.g., a new topology check) could easily be missed in the other, silently making build() and build_with_statistics() diverge in behavior.

Consider factoring the shared "validate + canonicalize" preamble into a helper that both build_with_kernel and build_with_kernel_and_statistics call before choosing which underlying construction function to invoke.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/delaunay/builder.rs` around lines 1667 - 1728, The Euclidean and
Canonicalized branches in build_with_kernel_and_statistics duplicate the same
validation/canonicalization preamble already present in build_with_kernel, so
the two build paths can drift. Extract the shared topology rejection, model
validation, and canonicalize_vertices setup into a helper used by both
build_with_kernel and build_with_kernel_and_statistics, then have each path only
choose between build_with_kernel_options and
build_with_kernel_options_and_statistics for the final construction call.

tests/semgrep/src/project_rules/rust_style.rs (1)

379-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extend legacy_delaunay_batch_constructors_bad to cover all 9 legacy variants.

Only 4 of the 9 name variants matched by the new no-legacy-delaunay-try-new-constructors regex (try_new_with_construction_statistics, try_new_with_topology_guarantee, try_with_topology_guarantee, try_with_topology_guarantee_and_options, try_with_options_and_statistics are missing) are exercised here. Adding the remaining cases would give full regex-coverage confidence for this new rule.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/semgrep/src/project_rules/rust_style.rs` around lines 379 - 399,
`legacy_delaunay_batch_constructors_bad` only exercises 4 of the 9 legacy
`DelaunayTriangulation` constructor variants covered by the new
`no-legacy-delaunay-try-new-constructors` regex. Extend this function to add the
missing legacy calls using the same pattern and `ruleid` annotations,
specifically covering `try_new_with_construction_statistics`,
`try_new_with_topology_guarantee`, `try_with_topology_guarantee`,
`try_with_topology_guarantee_and_options`, and
`try_with_options_and_statistics`, so the test validates full regex coverage.

examples/topology_editing.rs (1)

606-640: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Fine for a demo, but brute-force facet search clones the whole triangulation per candidate.

find_roundtrip_k2_facet_3d clones dt and attempts a forward+inverse Pachner move for every internal facet until one round-trips. For the small fixed 9-vertex fixture this is negligible, but it's worth keeping in mind if this helper is ever reused with larger inputs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@examples/topology_editing.rs` around lines 606 - 640, The helper
find_roundtrip_k2_facet_3d is doing a full dt.clone() and Pachner trial for
every candidate facet, which makes the search unnecessarily expensive. Refactor
the search to avoid cloning the whole triangulation per iteration—either
prefilter candidate facets more cheaply before trialing, or restructure the loop
so the expensive clone/propose_pachner/attempt_on path in
find_roundtrip_k2_facet_3d is only used on a much smaller set of likely internal
facets.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes